Skip to content

[fix][admin] Add SNI header when tlsHostnameVerification is not enabled - #17543

Merged
codelipenghui merged 2 commits into
apache:masterfrom
fantapsody:fix-admin-sni
Sep 14, 2022
Merged

[fix][admin] Add SNI header when tlsHostnameVerification is not enabled#17543
codelipenghui merged 2 commits into
apache:masterfrom
fantapsody:fix-admin-sni

Conversation

@fantapsody

Copy link
Copy Markdown
Contributor

Fixes #16416

Motivation

The pulsar admin client and the HTTP lookup service don't add an SNI header when tlsHostnameVerification is not enabled.

Modifications

The async-http-client doesn't split the flag for SNI header and hostname verification, so I added a new SSL engine factory to set the SNI header.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as (please describe tests).

Does this pull request potentially affect one of the following parts:

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@fantapsody

Copy link
Copy Markdown
Contributor Author

@michaeljmarshall PTAL

@fantapsody

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@michaeljmarshall michaeljmarshall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solution looks good to me, but it needs one minor fix (see comment).

The underlying issue is here:

  public SSLEngine newSslEngine(AsyncHttpClientConfig config, String peerHost, int peerPort) {
    SSLEngine sslEngine =
      config.isDisableHttpsEndpointIdentificationAlgorithm() ?
        sslContext.newEngine(ByteBufAllocator.DEFAULT) :
        sslContext.newEngine(ByteBufAllocator.DEFAULT, domain(peerHost), peerPort);
    configureSslEngine(sslEngine, config);
    return sslEngine;
  }

https://github.com/AsyncHttpClient/async-http-client/blob/c959fa0483adb4a71fbccc5be94d8b6faa4f74be/client/src/main/java/org/asynchttpclient/netty/ssl/DefaultSslEngineFactory.java#L63-L70

Specifically, when the engine is created without the hostname, the engine does not have the hostname to then pass as a header, which breaks the SNI routing.

An alternative solution could have overridden the newSslEngine method, but that seems equivalent to this one, so no need to change this PR.

Additionally, I was concerned that this would affect the case where hostname verification is not enabled and SNI is not used, but I think we should be fine, since this is just a header and we pass these headers when creating the client ssl engines for the Pulsar Protocol connections.

Thanks for opening the PR @fantapsody, I didn't understand the issue originally, but the PR helped me see it.

Comment thread pulsar-client/src/main/java/org/apache/pulsar/client/impl/HttpClient.java Outdated
@nodece

nodece commented Sep 9, 2022

Copy link
Copy Markdown
Member

I wonder can we use the allowTlsInsecureConnection() instead of the enableTlsHostnameVerification()?

@fantapsody
fantapsody requested review from michaeljmarshall and removed request for codelipenghui, eolivelli, jiazhai, lhotari and nodece September 9, 2022 13:48
@fantapsody

Copy link
Copy Markdown
Contributor Author

@michaeljmarshall Thanks for the feedback, please take another look.

@fantapsody

Copy link
Copy Markdown
Contributor Author

I wonder can we use the allowTlsInsecureConnection() instead of the enableTlsHostnameVerification()?

@nodece I think they do different validations.

@nodece nodece left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@michaeljmarshall michaeljmarshall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @fantapsody!

@michaeljmarshall

Copy link
Copy Markdown
Member

@fantapsody - can you please rebase this PR so that it picks up the latest CI fixes? There were recent changes that we need to capture before we can merge this PR. Thanks.

@fantapsody

Copy link
Copy Markdown
Contributor Author

@fantapsody - can you please rebase this PR so that it picks up the latest CI fixes? There were recent changes that we need to capture before we can merge this PR. Thanks.

Done. @michaeljmarshall

@codelipenghui codelipenghui added type/bug The PR fixed a bug or issue reported a bug area/admin labels Sep 14, 2022
@codelipenghui
codelipenghui merged commit 99b52eb into apache:master Sep 14, 2022
@codelipenghui codelipenghui modified the milestones: 2.12.0, 2.11.0 Sep 14, 2022
codelipenghui pushed a commit that referenced this pull request Sep 14, 2022
codelipenghui pushed a commit that referenced this pull request Sep 14, 2022
codelipenghui pushed a commit that referenced this pull request Sep 14, 2022
codelipenghui pushed a commit that referenced this pull request Sep 14, 2022
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Sep 14, 2022
@fantapsody
fantapsody deleted the fix-admin-sni branch September 14, 2022 12:38
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Sep 16, 2022
…ed (apache#17543)

(cherry picked from commit 99b52eb)
(cherry picked from commit 5cc6eeb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The pulsar-admin doesn't set SNI properly when tlsEnableHostnameVerification is false

4 participants